-
Notifications
You must be signed in to change notification settings - Fork 14
refactor(auth): remove redundant authentication checks #270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(auth): remove redundant authentication checks #270
Conversation
Summary by CodeRabbit
WalkthroughReplaced user lookups via get_current_user_info and AuthenticationFailed checks with direct use of request.user_id across task and task-assignment views. Updated all service/DTO calls to pass user_id=request.user_id. No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant JWT as JWT Middleware
participant TLV as TaskListView
participant TS as TaskService
Client->>JWT: Request /v1/tasks
JWT-->>Client: 401 if unauthenticated
JWT->>TLV: Forward request with request.user_id
TLV->>TS: listTasks(user_id=request.user_id)
TS-->>TLV: Tasks
TLV-->>Client: 200 OK (Tasks)
rect rgba(200, 230, 255, 0.3)
note over TLV: No get_current_user_info()<br/>No AuthenticationFailed in view
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've completed my review and didn't find any issues... but I did find this shark.
(`.
\ `.
) `._..---._
\`. __...---` o )
\ `._,--' , ___,'
) ,-._ \ ) _,-'
/,' ``--.._____\/--''
Files scanned
File Path | Reviewed |
---|---|
todo/views/task_assignment.py | ✅ |
todo/views/task.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
todo/views/task_assignment.py (1)
241-253
: Wrapspoc_id
in PyObjectId
Replace the rawrequest.user_id
withPyObjectId(request.user_id)
to matchAuditLogModel.spoc_id: PyObjectId | None
. Ensure you addfrom todo.models.common.pyobjectid import PyObjectIdand update:
- spoc_id=request.user_id, + spoc_id=PyObjectId(request.user_id),todo/views/task.py (3)
79-98
: Duplicateprofile
branch causes dead/unreachable code.The second
if query.validated_data["profile"]:
is redundant and never reached.Apply this diff to remove duplication and keep the richer response formatting:
if query.validated_data["profile"]: status_filter = query.validated_data.get("status", "").upper() response = TaskService.get_tasks_for_user( user_id=request.user_id, page=query.validated_data["page"], limit=query.validated_data["limit"], status_filter=status_filter, ) - return Response(data=response.model_dump(mode="json"), status=status.HTTP_200_OK) - - if query.validated_data["profile"]: - response = TaskService.get_tasks_for_user( - user_id=request.user_id, - page=query.validated_data["page"], - limit=query.validated_data["limit"], - ) - return Response( - data=response.model_dump(mode="json", exclude_none=True), - status=status.HTTP_200_OK, - ) + return Response( + data=response.model_dump(mode="json", exclude_none=True), + status=status.HTTP_200_OK, + )
100-111
: Normalizestatus
in non-profile path for parity.You uppercased
status
only in the profile path; apply same normalization here to avoid case-sensitive mismatches.Apply:
- status_filter = query.validated_data.get("status") + status_filter = (query.validated_data.get("status") or "").upper() or None
141-146
: Map Pydantic validation errors to 400 instead of 500 whencreatedBy
is invalid.
CreateTaskDTO
will raise PydanticValidationError
(e.g., invalid ObjectId). Current code turns that into a 500.Apply this diff and import alias:
@@ - try: - dto = CreateTaskDTO(**serializer.validated_data, createdBy=request.user_id) + try: + dto = CreateTaskDTO(**serializer.validated_data, createdBy=request.user_id) response: CreateTaskResponse = TaskService.create_task(dto) @@ - except ValueError as e: + except PydanticValidationError as e: + error_response = ApiErrorResponse( + statusCode=400, + message=ApiErrors.VALIDATION_ERROR, + errors=[{"detail": str(e)}], + ) + return Response( + data=error_response.model_dump(mode="json"), + status=status.HTTP_400_BAD_REQUEST, + ) + except ValueError as e: if isinstance(e.args[0], ApiErrorResponse): error_response = e.args[0] return Response( data=error_response.model_dump(mode="json"), status=error_response.statusCode, )Add near imports:
+from pydantic import ValidationError as PydanticValidationError
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
todo/views/task.py
(9 hunks)todo/views/task_assignment.py
(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Achintya-Chatterjee
PR: Real-Dev-Squad/todo-backend#52
File: todo/views/task.py:106-106
Timestamp: 2025-05-29T21:36:27.694Z
Learning: Issue #26 in the Real-Dev-Squad/todo-backend repository comprehensively tracks user authentication implementation including registration, login, JWT tokens, and making task APIs require authentication. This covers replacing hardcoded user ID placeholders like "system_patch_user" with actual user ID extraction from authenticated requests.
Learnt from: Achintya-Chatterjee
PR: Real-Dev-Squad/todo-backend#52
File: todo/views/task.py:106-106
Timestamp: 2025-05-29T21:36:27.694Z
Learning: Issue #26 tracks the implementation of user authentication in the todo-backend project, which includes extracting user ID from request context to replace hardcoded placeholders like "system_patch_user" in todo/views/task.py.
📚 Learning: 2025-05-29T21:36:27.694Z
Learnt from: Achintya-Chatterjee
PR: Real-Dev-Squad/todo-backend#52
File: todo/views/task.py:106-106
Timestamp: 2025-05-29T21:36:27.694Z
Learning: Issue #26 tracks the implementation of user authentication in the todo-backend project, which includes extracting user ID from request context to replace hardcoded placeholders like "system_patch_user" in todo/views/task.py.
Applied to files:
todo/views/task.py
todo/views/task_assignment.py
📚 Learning: 2025-05-29T21:36:27.694Z
Learnt from: Achintya-Chatterjee
PR: Real-Dev-Squad/todo-backend#52
File: todo/views/task.py:106-106
Timestamp: 2025-05-29T21:36:27.694Z
Learning: Issue #26 in the Real-Dev-Squad/todo-backend repository comprehensively tracks user authentication implementation including registration, login, JWT tokens, and making task APIs require authentication. This covers replacing hardcoded user ID placeholders like "system_patch_user" with actual user ID extraction from authenticated requests.
Applied to files:
todo/views/task.py
todo/views/task_assignment.py
🧬 Code graph analysis (2)
todo/views/task.py (4)
todo/services/task_service.py (2)
get_tasks_for_user
(687-700)delete_task
(680-684)todo/repositories/task_repository.py (1)
get_tasks_for_user
(353-361)todo/dto/task_dto.py (1)
CreateTaskDTO
(37-63)todo/services/task_assignment_service.py (2)
TaskAssignmentService
(18-152)create_task_assignment
(20-112)
todo/views/task_assignment.py (2)
todo/services/task_assignment_service.py (3)
TaskAssignmentService
(18-152)create_task_assignment
(20-112)delete_task_assignment
(148-152)todo/repositories/team_repository.py (2)
TeamRepository
(11-123)is_user_spoc
(108-115)
🔇 Additional comments (5)
todo/views/task_assignment.py (4)
60-60
: Confirmrequest.user_id
is always present and a valid ObjectId string.If middleware/permissions don’t run, this will raise AttributeError or fail downstream validations.
Would you like a small guard/mixin to enforce/annotate
request.user_id
on all auth’d views?
197-200
: LGTM: SPOC check now usesrequest.user_id
.This correctly delegates auth to middleware and enforces SPOC authorization here.
217-218
: Verify repository update semantics when switching team → user assignment.Ensure
update_assignment(task_id, executor_id, "user", request.user_id)
:
- Sets
assignee_id=executor_id
,- Switches
user_type
to"user"
,- Preserves/updates
updated_by
andupdated_at
.I can generate a quick repository-level check script if helpful.
289-289
: LGTM: propagaterequest.user_id
to delete.Consistent with the service contract and auditability.
todo/views/task.py (1)
82-86
: LGTM:request.user_id
plumbed through all task operations.Consistent with JWT middleware responsibility and service contracts.
Please confirm all these views run behind authentication/permission classes so
request.user_id
is guaranteed (e.g., globalDEFAULT_PERMISSION_CLASSES
or per-viewIsAuthenticated
).Also applies to: 107-110, 285-286, 295-296, 339-340, 426-427
Date: 03/08/2025
Developer Name: Shobhan Sundar Goutam (@shobhan-sundar-goutam)
Issue Ticket Number
TaskListView
#232Description
This PR removes redundant authentication checks (e.g.,
if not user: raise AuthenticationFailed(...)
). Authentication is now solely handled by the JWT middleware, which correctly returns 401 errors for unauthenticated requests.Summary
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes
Description by Korbit AI
What change is being made?
Remove redundant authentication checks and simplify user role assignments by replacing
RoleName
constants with direct string values for role names.Why are these changes being made?
The redundant authentication checks within the
task.py
andtask_assignment.py
views were removed to streamline request processing, asrequest.user_id
is already set by the middleware. Additionally, simplifying role assignment by using string values instead of constants reduces complexity and increases code readability. These adjustments help in reducing unnecessary code logic while maintaining functionality.